Yieldlab: Add Digital Service Act (DSA) handling #3473
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Code coverage summaryNote:
yieldlabRefer here for heat map coverage report |
Mufas61
force-pushed
the
Yieldlab_Adapter_Add_DSA_support
branch
from
36b8add
to
b294d92
Compare
Code coverage summaryNote:
yieldlabRefer here for heat map coverage report |
bsardo
changed the title
Code coverage summaryNote:
yieldlabRefer here for heat map coverage report |
Code coverage summaryNote:
yieldlabRefer here for heat map coverage report |
Code coverage summaryNote:
yieldlabRefer here for heat map coverage report |
Code coverage summaryNote:
yieldlabRefer here for heat map coverage report |
|
lgtm |
brushmate
approved these changes
Feb 13, 2024
bsardo
reviewed
Feb 13, 2024
| q.Set("dsapubrender", strconv.Itoa(*dsa.PubRender)) | ||
| } | ||
| if dsa.DataToPub != nil { | ||
| q.Set("dsadatatopub", strconv.Itoa(*dsa.DataToPub)) |
There was a problem hiding this comment.
It is unclear to me as to what this parameter is supposed to be. I'm guessing what you have here is correct but I'm trying to confirm. In Object Specification for OpenRTB 2.X it is datatopub and in URL-based support it is dsadatapubs.
I would have expected the two to be the same with maybe the URL-based parameter having a dsa prefix that the object field does not.
There was a problem hiding this comment.
This is indeed correct. The have prefixed all our DSA URL parameters, but saw no reason why this parameter should be plural, even though this might be against the current spec.
adapters/yieldlab/yieldlab.go
Outdated
| fmt.Errorf("failed to add Yieldlab DSA object for adslotID %v. This is most likely a programming issue", bid.ID), | ||
| } | ||
| } | ||
| responseBid.Ext = dsaJson |
There was a problem hiding this comment.
I just want to confirm that you want to overwrite the entire bid.ext with the DSA object.
There was a problem hiding this comment.
Yes, bid.ext should not be defined somewhere else. I refactored this part to make the intention clearer.
| transparencies []dsaTransparency | ||
| expected string | ||
| }{ | ||
| { |
There was a problem hiding this comment.
You might want to consider adding two more test cases for when transparencies is nil and when Params is nil.
Comment on lines
113
to
114
| "dsa": { | ||
| } |
There was a problem hiding this comment.
DSA logic has not made its way into PBS core yet but I wanted to shed light on how we are planning to implement it as it might have an impact on the changes you're making here. When dsarequired is set to 2 or 3 in the incoming request, the object seatbid.bid.ext.dsa will need to be present for a given bid, otherwise the bid will be discarded by PBS core. An empty DSA object, as opposed to a nil or missing object, as you have here is considered present so the bid won't be discarded.
Comment on lines
+327
to
+336
| func Test_getDSA_invalidRequestExt(t *testing.T) { | ||
| req := &openrtb2.BidRequest{ | ||
| Regs: &openrtb2.Regs{Ext: json.RawMessage(`{"DSA":"wrongValueType"}`)}, | ||
| } | ||
|
|
||
| dsa, err := getDSA(req) | ||
|
|
||
| assert.NotNil(t, err) | ||
| assert.Nil(t, dsa) | ||
| } |
There was a problem hiding this comment.
Error cases like this should be covered using the JSON test framework wherever possible. To do so, you can create a new directory yieldlabtest/supplemental with a new JSON test in it (e.g. invalid-reg-ext) that has an invalid reg.ext in the mockBidRequest:
"regs": {
"ext": {
"dsa": ""
}
}
In your JSON test you declare the expected MakeRequests error as such:
"expectedMakeRequestsErrors": [{
"value": "failed to parse Regs.Ext object from Yieldlab response: json: cannot unmarshal string into Go struct field openRTBExtRegsWithDSA.dsa of type yieldlab.dsaRequest",
"comparison": "literal"
}]
|
@Mufas61 @brushmate I just want to bring your attention to my comments left here because I recognize the time sensitive nature of this PR. |
Code coverage summaryNote:
yieldlabRefer here for heat map coverage report |
Thanks, I've just pushed some changes that address your other comments. Would be really great if we could get this change released before the weekend (Feb 17th). |
bsardo
approved these changes
Feb 14, 2024
brushmate
approved these changes
Feb 14, 2024
VeronikaSolovei9
approved these changes
Feb 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of change
Handling of the DSA information in the request and response.
We had to implement this even though there is no official handling in the prebid-server yet, since the DSA law will apply as of February 17th. Therefore, there are some things on adapter level, like the DSA object, that should normally be implemented differently. We followed what was discussed in Issue#3424.
Other Information
A review and approval from a member of the Yieldlab team are required. (@brushmate, @rey1128, @nkloeber)